Repository navigation
[OMNIML-6228] Shrink the input of the conv/IsInf ONNX test model - #2947
Merged
Merged
Conversation
build_conv_isinf_model builds a 6x32x900x256 fp32 input (177 MB per activation, and the
Conv/Cast/IsInf/Where chain keeps several of them), but the three tests that use it check node
dtypes and QDQ placement, never values. Measured as the peak summed RSS of the pytest process tree
(imports included) on a CPU host, test_autocast.py peaks at 2.6 GB and test_qdq_rules_int8.py at
2.3 GB; with a 1x32x64x64 input they peak at 1.3 GB and 1.5 GB and run 3.5 s and 1.6 s faster.
The assertions are unchanged.
Under pytest-xdist the memory adds up: four workers run files at once, and each worker that has
used cpu_dist_workers keeps its two-rank pool alive. Of the 251 unit test files run alone, these
two are the 9th and 10th largest; the eight above them (2.9-3.6 GB) are six files that use the
pool, test_gptq.py (it spawns its own processes) and test_distill.py. A transient spike of 0.8-1.2 GB
in one worker is what tips a 4-vCPU runner over: the hfep PRs' torch_28 unit jobs were stopped by
the runner ("received a shutdown signal") with these tests in flight in 9 of 33 runs.
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: reducing the input size of the conv/IsInf ONNX test model. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Security Anti-Patterns PASS. The pull request changes only tests/_test_utils/onnx/lib_test_models.py, outside themodeloptpackage andexamplesPython paths covered by this check. The diff only changes ONNX test tenso…Full details: Security Anti-Patterns
-
Explanation
PASS. The pull request changes only
tests/_test_utils/onnx/lib_test_models.py, outside themodeloptpackage andexamplesPython paths covered by this check. The diff only changes ONNX test tensor shapes and adds a comment. It adds notorch.load,numpy.load,trust_remote_code,eval,exec,# nosec, or dependency changes.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2947 +/- ##
==========================================
+ Coverage 70.08% 78.18% +8.10%
==========================================
Files 659 659
Lines 74089 77529 +3440
==========================================
+ Hits 51923 60619 +8696
+ Misses 22166 16910 -5256
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
shengliangxu
marked this pull request as ready for review
October 22, 2026 20:21
meenchen
reviewed
Oct 22, 2026
meenchen
left a comment
meenchen
left a comment
Contributor
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
LGTM: the smaller fixture preserves graph topology and all dtype/QDQ assertions; human sign-off is required for the justified existing-test fixture edit.
Needs action:
- Confirm the fixture-size reduction in
tests/_test_utils/onnx/lib_test_models.py; all three callers check structure and precision, not large-shape behavior.
meenchen
approved these changes
Oct 22, 2026
Contributor
|
This was referenced Oct 23, 2026
shengliangxu
added a commit
that referenced
this pull request
Oct 27, 2026
…rkers (#3064) ### What does this PR do? Type of change: CI (fewer runner stops in the unit job) `test_gptq_hessian_dp_sync` spawned its own two-process gloo group (`spawn_multiprocess_job`) next to the persistent two-rank pool that `cpu_dist_workers` keeps for the other multi-rank unit tests. This PR runs the same job on the pool (`cpu_dist_workers.run`); the test body is unchanged. **Why.** The `torch_28` unit job (16 GB hosted runner) is stopped by the runner ("The runner has received a shutdown signal"), and this test is in flight every time. These are not the stops #2947 fixed (the conv/IsInf ONNX tests, mid-run). | `multi-version (unit-3.12(torch_28, tf_latest))`, attempts since 10-26 00:00 UTC | | |---|---| | finished attempts | 426 | | runner stops | 13 (3.1%) | | progress of the run at the stop | 97 to 99% | | `test_gptq_hessian_dp_sync` in flight at the stop | 13 of 13 | **Effect.** With a live pool the spawn adds two processes that re-import torch and modelopt; the pooled job adds none: | `test_dist.py::test_data_parallel` (builds the pool), then the dp-sync test; local CPU, two runs each | processes at the peak | peak RSS | peak PSS | wall time | |---|---|---|---|---| | before: spawn | 6 | 5.35 and 5.38 GiB | 3.76 and 3.76 GiB | 72 and 58 s | | after: pool | 4 | 3.26 and 3.24 GiB | 2.42 and 2.43 GiB | 44 and 45 s | The saving applies when the xdist worker that runs the test already has a live pool; if it has none, the pool is built instead of the spawn (the same cost) and stays for the rest of the run. ### Testing - `tests/unit/torch/quantization/test_gptq.py`: 7 passed; run together with `test_dist.py` (which shares the pool): passes. - Memory and time: the table above. - Not run: the full `torch_28` unit job locally. Whether the stop rate drops is seen in the `torch_28` attempts of the next days. ### Before your PR is "*Ready for review*" Make sure you read and follow [Contributor guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md) and your commits are signed (`git commit -s -S`). Make sure you read and follow the [Security Best Practices](https://github.com/NVIDIA/Model-Optimizer/blob/main/SECURITY.md#security-coding-practices-for-contributors) (e.g. avoiding hardcoded `trust_remote_code=True`, `torch.load(..., weights_only=False)`, `pickle`, etc.). - Is this change backward compatible?: N/A (unit test only) - If you copied code from any other sources or added a new PIP dependency, did you follow guidance in `CONTRIBUTING.md`: N/A - Did you write any new necessary tests?: N/A (changes how an existing test runs) - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: N/A (CI) - Did you get Claude approval on this PR?: N/A (not run yet) ### Additional Information Jira: [OMNIML-6228] (flaky-test cleanup, the `torch_28` stops). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Tests** * Updated the distributed GPTQ Hessian synchronization test to use the standard CPU worker setup and cover uneven token counts, including a worker with zero tokens. This change affects test execution only; no user-facing functionality was changed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Type of change: Bug fix (tests)
build_conv_isinf_modelbuilds a 6x32x900x256 fp32 input (177 MB per activation, and the Conv/Cast/IsInf/Where chain keeps several). The three tests that use it (test_conv_isinf_conversion[13|21]intests/unit/onnx/autocast/test_autocast.py,test_conv_isinf_int8intests/unit/onnx/quantization/test_qdq_rules_int8.py) check node dtypes and QDQ placement, never values. The input is now 1x32x64x64; the assertions are unchanged.Measured as the peak summed RSS of the pytest process tree (imports included) on a CPU host, run per file:
test_autocast.pytest_qdq_rules_int8.pyWhy it matters: since the unit tests run under pytest-xdist (#2845), four workers run files at once on a 4-vCPU runner, and each worker that has used
cpu_dist_workerskeeps a two-rank pool alive. Across the 251 unit files run alone, these two are the 9th and 10th largest (the eight above them are six pool-based quantization/export files,test_gptq.pyandtest_distill.py). Themulti-version (unit-3.12(torch_28, ...))job has been stopped by the runner ("The runner has received a shutdown signal", no failed test) 22 times since xdist merged: 15% of its 146 runs, 4 of 65 on branches other than the hfep stack, and no other torch version. It was reported on hfep #2752 five attempts in a row. These stops come from the whole job's memory (the CI speed-up work measured 14,608-15,708 MiB at the peak with--dist loadfileand is working on the structural part, the pool kept per worker), so this PR removes one 0.8-1.2 GiB spike, not the whole cause.Usage
N/A
Testing
tests/unit/onnx/autocast/test_autocast.pyandtests/unit/onnx/quantization/test_qdq_rules_int8.py: 31 passed on current main with the change applied.Before your PR is "Ready for review"
CONTRIBUTING.md: N/ASummary by CodeRabbit